-
Couldn't load subscription status.
- Fork 10.6k
Typos batch: lib and include folders
#75020
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Typos batch: lib and include folders
#75020
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of these changes aren't right and should be fixed.
|
@swift-ci Please test |
|
@swift-ci Please test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @Sajjon 🙏🏽 I reviewed the following directories:
include/swift/IDEinclude/swift//IDEToolinclude/swift/Parseinclude/swift/Refactoringlib/ASTGenlib/IDElib/IDEToollib/Macroslib/Parselib/Refactoring
|
@swift-ci Please smoke test |
lib/SIL/IR/SILPrinter.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AnthonyLatsis should revert, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can change it, but in this case you have to change all occurrences in all test files, too. If you want to do this, I recommend to put those changes in a separate commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would extract that change into a separate PR, this one is quite large already. If you choose to revert, please make sure to audit all modified occurrences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm leaving it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then please put everything related to dynamically_replaceable in a separate commit, and make sure to modify all occurrences across the repository. Otherwise the tests will not pass, and we don’t want comments and documentation that mention this compiler attribute to be out-of-date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AnthonyLatsis I've changed all occurrence to -> dynamically_replacable in separate commit: 08ba5a5
This comment has been minimized.
This comment has been minimized.
|
I feel uncomfortable with the size and contents of this patch. There are so many changes that none of the reviewers are going to read the entire patch, which would make it really easy for something slip through review here. Since none of these changes depend on each other, would you mind splitting this up into many smaller PRs that either all fix the same typo, or fix only typos in one directory so there's a smaller set of reviewers for each PR? |
Fix typos to
libandincludefolders, as per suggestion by @compnerdIf valuable, I did split the work up initially, in my fork, in two PRs merged into this source branch, both of which, in turn was aggregated from several separate PRs.
lib/, merged from:lib/IRGenlib/Sema&lib/Refactoringlib/SILOptimizerlib/AST & lib/ASTGenlib/IDElib/sil&lib/SILGenlib/* (misc)include/, merged from:include/swift/SIL&include/swift/SILOptimizerinclude/swift/ASTinclude/*(misc)(most likely we wanna squash those 22 commits into a single one...)